fix(open): set wait: false to run server.close successfully
#2001
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2001 +/- ##
=======================================
Coverage 92.77% 92.77%
=======================================
Files 29 29
Lines 1149 1149
Branches 327 327
=======================================
Hits 1066 1066
Misses 79 79
Partials 4 4
Continue to review full report at Codecov.
|
|
@evilebottnawi I added one commit(fdc6d31). PTAL |
lib/utils/runOpen.js
Outdated
| openOptions = { app: options.open }; | ||
| Object.assign(openOptions, { | ||
| app: options.open, | ||
| }); |
There was a problem hiding this comment.
let openOptions = { wait: false };
// ...
if (typeof options.open === 'string') {
openOptions = Object.assign({} openOptions, { app: options.open });
}No need modify openOptions variable, it is very bad practice
There was a problem hiding this comment.
openOptions = Object.assign({} openOptions, { app: options.open }); equals Object.assign(openOptions, { app: options.open }).
I don't think this is a very bad practice.
There was a problem hiding this comment.
Yes, but it is not good, we modify original object and it is bad practice because you can forget about this in future and it is lead to some problems when you refactor code.
When you pass options to third party package(s) better always cloning object. Other package can modify object too so it can lead to very strange errors and situations.
There was a problem hiding this comment.
When you pass options to third party package(s) better always cloning object. Other package can modify object too so it can lead to very strange errors and situations.
If you say, we should not assign to the same variable.
I don't have any strong opinions, so I'll change.
hiroppy
force-pushed
the
feature/fix-open
branch
from
a64edce to
abeaa9c
Compare
|
@evilebottnawi PTAL |
fixes: #1990
For Bugs and Features; did you add new tests?
yes
Motivation / Use-Case
see #1990
commit: sindresorhus/open@da2d663#diff-168726dbe96b3ce427e7fedce31bb0bcR23
Our open(opn)'s version is old, and
waitseems to betrueby default in this version.Breaking Changes
no
Additional Info